Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement/issue 427 css bundling rollup #438

Merged

Conversation

thescientist13
Copy link
Member

@thescientist13 thescientist13 commented Dec 1, 2020

Related Issue

resolves #427

Summary of Changes

  1. Introduces PostCSS bundling and optimizations in rollup.config.js (including filename hashing)
  2. Removed manual copy step from copy.js lifecycle (all <link> gets inferred via Rollup now).
  3. prism.css now works! 🎉
  4. Removed theme.css config option and updated / enabled style related test cases, since users can now just bring their own <style> and <link> tags in their templates

Screen Shot 2020-12-01 at 3 51 01 PM

Notes

The following will be resolved by #426

  • custom project postcss.config.js
  • support for nested selectors in www/ CSS

turns out rollup plugin postcss seems to only be for CSS-in-JS (e.g. import 'something.css';)

  • that's what generates style-inject.es..js
  • support for this should be moved to external plugin post feature parity is if there's JS loaded, then there will be no styles.
    should recommend "regular" CSS, or can we inline / inject / extract them all maybe?

TODO

  1. For some reason, can't get emitFiles to work (only) when running test cases. No CSS files are getting output to public/ :/
  2. Confirm: Seems like there is no real need to support theme.css anymore

@thescientist13 thescientist13 added enhancement Improve something existing (e.g. no docs, new APIs, etc) CLI labels Dec 1, 2020
@thescientist13
Copy link
Member Author

Ok, so trying out some things and by making the whole buildStart hook synchronous, I was able to actually get the emitted CSS files showing up in Rollups bundles in the generateBundle hook.

    generateBundle(outputOptions, bundles) {
      const mappedBundles = new Map();
      const that = this;
      console.debug('rollup generateBundle bundles', Object.keys(bundles));
    }

and get this for the console output

rollup generateBundle bundles [
  'styles/theme.1607212623838.css',
  '.greenwood/index.55edcbe0.js',
  '.greenwood/blog/first-post/index.c188eb5c.js',
  '.greenwood/blog/second-post/index.0c992a1d.js',
  'footer.3e1dc649.js',
  'header.c610a1c8.js'
]

BUT, then I get this error from Rollup

TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received type undefined
    at validateString (internal/validators.js:112:11)
    at Object.resolve (path.js:981:7)
    at writeOutputFile (/Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/node_modules/rollup/dist/shared/rollup.js:20057:30)
    at /Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/node_modules/rollup/dist/shared/rollup.js:20000:65
    at Array.map (<anonymous>)
    at handleGenerateWrite (/Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/node_modules/rollup/dist/shared/rollup.js:20000:50)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at async /Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/packages/cli/src/lifecycles/bundle.js:2:233 {
  code: 'ERR_INVALID_ARG_TYPE'

And then I notice that the type of bundle for the CSS file is actually placeholder? I would have assumed asset. 🤔

bundles [Object: null prototype] {
  'styles/theme.1607212623838.css': { type: 'placeholder' },
  '.greenwood/index.55edcbe0.js': {
    exports: [],
    facadeModuleId: '/Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/packages/cli/test/cases/build.default.workspace-getting-started/.greenwood/index.html',
    isDynamicEntry: false,
    isEntry: true,
    isImplicitEntry: false,
    ...

No amount of Googling has really helped so far though... 🤷

Will see what happens, but will likely want to make an issue to specifically tracking this if I can't come to a resolution by the end of the week, so I can keep working on other things and testing our work so far via some alpha testing.

@thescientist13
Copy link
Member Author

thescientist13 commented Dec 10, 2020

I figured out the issue, I think it is with this postcss code, in particular the .async bit?

postcss(postcssConfig.plugins)
  .use(postcssImport())
  .process(source, { from: filePath })
  .async();

Hardcoded that just to a string and omitting it and now files are getting emitted correctly. 💥

that.emitFile({
  type: 'asset',
  fileName,
  name: href,
  source: '.some-css { }'
});

Just need to figure out a way around not using .async and should be good to go! 🥳

edit: it's specifically related to the nano plugin being async, so will need to figure out a couple more things, but at least now it is behaving as expected output wise.

@thescientist13 thescientist13 self-assigned this Dec 10, 2020
Copy link
Member Author

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figured out the issue with the tests and was definitely due to async behavior within buildStart in rollup.config.js. It seems PostCSS will only support being run asynchronously so when mixed with sync code like the parser and rollup (this.emitFile) we were hitting all kinds of nasty race conditions.

So a little clunky of a solution for now, but at least reliable. Certainly room for improvement we can do via another PR.

Copy link
Member Author

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed support for theme.css config since it's not really needed now since any can BYOC (Bring your own CSS) and they already own the HTML file anyway. win-win, so just updated and enabled some additional test cases. This is all good to go now. 💯

@thescientist13 thescientist13 merged commit 6e4e974 into release/0.10.0 Dec 13, 2020
thescientist13 added a commit that referenced this pull request Apr 3, 2021
* init draft of CSS bundling with rollup and moved out of copy lifecycle

* emit and create CSS files in one place

* manually processing CSS with PostCSS and got cssnano minification working

* dont emit already emitted CSS link assets

* address PostCss from option warning

* disable theme.css tests in getting started case

* convert postcss to async

* CSS filename hashing

* css bunding and support for relative @imports

* remove logging

* refactoring

* temp work around for PostCSS and Rollup

* delete describe.only

* restored file name content hashing

* aysnc refactor around PostCSS

* added test cases for style and link tags, removed theme.css config

* refine style test cases
thescientist13 added a commit that referenced this pull request Apr 3, 2021
* init draft of CSS bundling with rollup and moved out of copy lifecycle

* emit and create CSS files in one place

* manually processing CSS with PostCSS and got cssnano minification working

* dont emit already emitted CSS link assets

* address PostCss from option warning

* disable theme.css tests in getting started case

* convert postcss to async

* CSS filename hashing

* css bunding and support for relative @imports

* remove logging

* refactoring

* temp work around for PostCSS and Rollup

* delete describe.only

* restored file name content hashing

* aysnc refactor around PostCSS

* added test cases for style and link tags, removed theme.css config

* refine style test cases
@thescientist13 thescientist13 deleted the enhancement/issue-427-css-bundling-rollup branch July 20, 2022 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI enhancement Improve something existing (e.g. no docs, new APIs, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant